Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added CPU architecture detection and option #101

Merged
merged 9 commits into from
Dec 8, 2024
Merged

Added CPU architecture detection and option #101

merged 9 commits into from
Dec 8, 2024

Conversation

michael-picard
Copy link
Contributor

This PR adds the ability to pass --arch=arm64 option when installing drivers.
The cpu architecture is detected automatically in DriverFactory::createFromBrowser().
As of today, there is no chromedriver for Linux and arm64 :

dbrekelmans
dbrekelmans requested changes Nov 23, 2024
View reviewed changes
Copy link
Owner

@dbrekelmans dbrekelmans left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a few small suggested changes and I'll be happy to merge it 💪
Take a look at the CI as well and make sure the build succeeds.

Thanks for the contribution!

src/Driver/ChromeDriver/Downloader.php Outdated Show resolved Hide resolved
tests/Driver/ChromeDriver/DownloadUrlResolverTest.php Outdated Show resolved Hide resolved
src/Cpu/CpuArchitecture.php Outdated Show resolved Hide resolved
src/Cpu/CpuArchitecture.php Outdated Show resolved Hide resolved
@dbrekelmans
Copy link
Owner

@michael-picard Hi, just pinging you in case you didn't get a notification of my review. Good work so far, just a few small changes required!

michael-picard and others added 2 commits November 29, 2024 09:24
@michael-picard @dbrekelmans
Update src/Driver/ChromeDriver/Downloader.php
7e8e571 
Co-authored-by: Daniël Brekelmans <dbrekelmans@users.noreply.github.com>
@michael-picard @dbrekelmans
Update src/Cpu/CpuArchitecture.php
4b3ecd0 
Co-authored-by: Daniël Brekelmans <dbrekelmans@users.noreply.github.com>
@michael-picard
Copy link
Contributor Author

Hi @dbrekelmans sorry fo the late reply, I got swamped this week, I'm gonna work on this today.

@michael-picard
Copy link
Contributor Author

@dbrekelmans I tried to run the pull_request CI locally using act but it fails on php bdi browser:google-chrome -vvv with the error 'google-chrome: not found'. It would seem "normal" to me since the ca.yml file doesn't install any browsers. Am I misunderstanding something here? Sorry I'm still fairly new in Github actions.

@dbrekelmans
Copy link
Owner

If you could just resolve the PHPStan and PHPCS failures, I'll take care of the QA checks after merging.

You can run these tools locally by running vendor/bin/phpstan analyse and vendor/bin/phpcs --warning-severity=0

@michael-picard
Copy link
Contributor Author

Hi @dbrekelmans , it's done.

@michael-picard
Copy link
Contributor Author

Hi @dbrekelmans I reran phpcbf for browser-driver-installer/src/Cpu/CpuArchitecture.php, looks like the volume mounting of Docker was getting in the way, I had to do remove the extra space manually.

@dbrekelmans dbrekelmans merged commit 8422fe3 into dbrekelmans:main Dec 8, 2024
12 of 15 checks passed
@dbrekelmans
Copy link
Owner

Thanks @michael-picard!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbrekelmans dbrekelmans dbrekelmans requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@michael-picard @dbrekelmans